Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add a warning for when the history path doesn't exist #852

Merged
merged 4 commits into from
Feb 2, 2024
Merged

Add a warning for when the history path doesn't exist #852

merged 4 commits into from
Feb 2, 2024

Conversation

ignacio-chiazzo
Copy link
Contributor

@ignacio-chiazzo ignacio-chiazzo commented Jan 31, 2024 •
edited
Loading

When using a file path that doesn't exist in the IRB.conf[:HISTORY_FILE] env variable, IRB creates a new file with that directory without warning the developer.

This PR prints a warning when that happens.

➜  irb git:(add-warning) ✗ echo 'IRB.conf[:HISTORY_FILE] = "/Users/ignaciochiazzo/irb/test/irb/fake-folder/fake/testing.rb"' > ~/.irbrc

➜  irb git:(add-warning) ✗ bundle exec irb
irb(main):001> a = 1
=> 1
irb(main):002> exit
Warning: The path to save IRB history does not exist. Please, set the env var IRB.conf[:HISTORY_FILE] with a valid path.

#830

Demo

Screen.Recording.2024-02-02.at.4.31.29.PM.mov

@st0012
Copy link
Member

st0012 commented Jan 31, 2024

Can you add a test case to IRBHistoryIntegrationTest?

@st0012
Copy link
Member

st0012 commented Jan 31, 2024

Also, I don't think HISTORY_FILE env var would be picked up by IRB. Are you sure the example is working? Maybe you also set the path in .irbrc?

@tompng
Copy link
Member

tompng commented Feb 1, 2024 •
edited
Loading

Issue #830 is for no such directory case. Not the case that directory exists and history file does not exist.

st0012
st0012 reviewed Feb 1, 2024
View reviewed changes
lib/irb/history.rb Outdated Show resolved Hide resolved
lib/irb/history.rb Outdated Show resolved Hide resolved
@ignacio-chiazzo ignacio-chiazzo force-pushed the add-warning branch 2 times, most recently from 0458b7b to 05dfbd1 Compare February 2, 2024 19:26
st0012
st0012 reviewed Feb 2, 2024
View reviewed changes
lib/irb/history.rb Outdated Show resolved Hide resolved
st0012
st0012 approved these changes Feb 2, 2024
View reviewed changes
Copy link
Member

@st0012 st0012 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much 👍

@st0012 st0012 added the bug Something isn't working label Feb 2, 2024
@st0012 st0012 merged commit 9e6fa67 into ruby:master Feb 2, 2024
29 checks passed
matzbot pushed a commit to ruby/ruby that referenced this pull request Feb 2, 2024
@ignacio-chiazzo @st0012 @matzbot
[ruby/irb] Add a warning for when the history path doesn't exist
aa780a6 
(ruby/irb#852)

* Add a warning for when the history path doesn't exist

* warn when the directory does not exist

* added test for when the history_file does not exist

* Update lib/irb/history.rb

---------

ruby/irb@9e6fa67212

Co-authored-by: Stan Lo <stan001212@gmail.com>
@st0012 st0012 mentioned this pull request Feb 2, 2024
Merged
@nobu
Copy link
Member

nobu commented Feb 3, 2024 •
edited
Loading

This causes a warning in ruby/ruby:

Warning: The directory to save IRB's history file does not exist. Please double check `IRB.conf[:HISTORY_FILE]`'s value.

nobu added a commit to nobu/irb that referenced this pull request Feb 3, 2024
@nobu
Consume the warning for non-existent history path
9a7e060 
Fix ruby#852 (comment)
matzbot pushed a commit to ruby/ruby that referenced this pull request Feb 3, 2024
@nobu @matzbot
[ruby/irb] Consume the warning for non-existent history path
124be0a 
Fix ruby/irb#852 (comment)

ruby/irb@9a7e060e57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@st0012 st0012 st0012 approved these changes

@tompng tompng Awaiting requested review from tompng

Assignees
No one assigned
Labels
bug Something isn't working
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ignacio-chiazzo @st0012 @tompng @nobu